Skip to content

Conversation

@mail4umar
Copy link
Collaborator

Previous "Execution Time" in the Summary tab was a bit misleading.
Now we are adding three times:

  • Completion Time (total time since requested by user)
  • Queue Time (time in queue)
  • Run Time (the time actually taken by query to run including planning)

@mail4umar mail4umar requested a review from lcoffinOT June 17, 2025 13:27
@mail4umar mail4umar self-assigned this Jun 17, 2025
Copy link
Collaborator

@lcoffinOT lcoffinOT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to add a new test case validating the new timing fields. Maybe check that they're always positive values (or 0), and that queue time is less than completion time.

Copy link
Collaborator

@lcoffinOT lcoffinOT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mail4umar thanks for the update! I still think we should include new test cases with this PR as well (see my previous review comment #1350 (review) ). If this needs review again while I'm out, can you pull Danny in?

@mail4umar
Copy link
Collaborator Author

Might want to add a new test case validating the new timing fields. Maybe check that they're always positive values (or 0), and that queue time is less than completion time.

This will be a bit problematic. Because we will not create a scenario where the resources are strained and there is a queue time.

In the code it is only pulling out the values from the resource_acq table. I am sure that table will have checks in place for making sure that there are no negative values etc.

@mail4umar mail4umar requested a review from DMickens June 20, 2025 10:36
@DMickens
Copy link
Contributor

Might want to add a new test case validating the new timing fields. Maybe check that they're always positive values (or 0), and that queue time is less than completion time.

My thought is that we are pulling directly from Vertica for this data. If this test failed, it would point to an issue in Vertica, not qprof. For that reason I believe that test would be unnecessary

@lcoffinOT
Copy link
Collaborator

Looks good to me, but seems like it's failing the tests now? Or am I reading the 'Some checks were not successful' box wrong?

@lcoffinOT lcoffinOT closed this Jun 30, 2025
@lcoffinOT
Copy link
Collaborator

I did not mean to close this 😅

@lcoffinOT lcoffinOT reopened this Jun 30, 2025
@mail4umar mail4umar merged commit 2a49f03 into vertica:master Jul 1, 2025
5 of 25 checks passed
@mail4umar mail4umar deleted the qprof_summary_update branch July 1, 2025 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants